Some issues come to mind.

• I've now introduced CPU-specific code to MMTk, I don't know if we have a plan for how to put that in the codebase. My current approach of dumping #[cfg(target_arch = "x86_64")] in the Compressor directory is rather undisciplined.
• ranges::break_byte_range is patterned after ranges::break_bit_range; I don't know if that's a good approach, and it's currently undocumented and untested.
• I have a portable prefix sum in forwarding::prefix_sum — although according to the Wikipedia article it's a xor-scan, although xor is addition over ℤ₂ if you're so inclined — which appears about as fast as the original branchy algorithm. Is it worthwhile to keep the original branchy code, or should the fallback use that prefix sum algorithm?
• I'm still waiting for properly-measured results, but so far the carryless multiply algorithm is either within the noise or up to 5% faster in STW time than the branchy algorithm. This might not be a large enough speedup to deserve the complexity.

Plotty and here are the geomean results for clmul-enabled relative to clmul-disabled:

Plotty and here are the geomean results for clmul-enabled relative to clmul-disabled:
Heap factor Total time STW time
1000 0.982 0.970
1416 0.997 0.984
1892 0.989 0.970
2428 0.997 0.983
3023 0.996 0.980
3678 0.996 0.977
4392 0.994 0.970
5166 0.995 0.981
6000 1.001 0.965

To clarify, do both builds include the change for computing multiple regions in one CalculateOffsetVector package? Just asking, as the PR has two changes (clmul and the work package change), and the performance impact of the work package change was not mentioned.

To clarify, do both builds include the change for computing multiple regions in one CalculateOffsetVector package? Just asking, as the PR has two changes (clmul and the work package change), and the performance impact of the work package change was not mentioned.

Yes, both are with this pull request, with different values for MMTK_COMPRESSOR_USE_CLMUL. I haven't ran systemic tests for the new work packets, but I found that one region* per work packet was too small and scheduling overhead dominated the offset vector phase. Here is a trace for lusearch with the old work packets, for example:

and with the new work packets:

*I also neglected to mention that I shrunk regions from 1MiB to 256kiB, to improve work balancing. The original 1MiB size was more-or-less arbitrary; and I found that the regions near the start of the heap would often accumulate long-lived objects, and so the Compact phase would spend dramatically more time on these regions. (The offset vector phase would previously have spent more time on these regions too, but the time of the new branch-free algorithm is proportional to the region size and not the live data.)

I also neglected to mention that I shrunk regions from 1MiB to 256kiB, to improve work balancing.

I tested this PR against upstream; Plotty and a tally of changes to stop-the-world times:

Some benchmarks see large improvements due to the better work balancing, some see small regressions due to worse locality with smaller region sizes. (I only found the mutator time to be consistently worse on tradesoap, which also has the worst STW regression at 5.4% slower.)

I also inadvertently wrote a AddressIterator which is basically a mmtk::vm::slot::AddressRangeIterator except that AddressIterator can iterate over any size and not just words. I don't think it makes much sense for mmtk::util::metadata::side_metadata::scan_words to depend on a type in mmtk::vm::slot, so I'm not sure how to unify the two.

For now I'll make AddressRangeIterator wrap AddressIterator as to not break API.

I also inadvertently wrote a AddressIterator which is basically a mmtk::vm::slot::AddressRangeIterator except that AddressIterator can iterate over any size and not just words. I don't think it makes much sense for mmtk::util::metadata::side_metadata::scan_words to depend on a type in mmtk::vm::slot, so I'm not sure how to unify the two.

For now I'll make AddressRangeIterator wrap AddressIterator as to not break API.

We could have just done (addr1..addr2).step_by(BYTES_IN_ADDRESS) if we could implement std::iter::Step for Address. Range<T> implements Iterator only if T implements Step. See https://doc.rust-lang.org/std/ops/struct.Range.html#impl-Iterator-for-Range%3CA%3E

I think what we have done is just a workaround for the unstable Rust API. Maybe we just manually implement IntoIterator<IntoIter=AddressIterator> for Range<Address>. Well, that could be done in its own PR.

• ranges::break_byte_range is patterned after ranges::break_bit_range; I don't know if that's a good approach, and it's currently undocumented and untested.

Yeah, I don't like this approach of breaking a range into words and bytes; this (rather accurate to the paper) take on the Compressor always uses metadata which is word-aligned with SideMetadataSpec::scan_words. The Android Runtime and Clozure Common Lisp garbage collectors, which use a mark bitmap differently to the paper, end up breaking ranges into words and bit ranges in words of metadata. I'm going to implement that style of mark bitmap next, but I don't want to upstream an interface that I'm going to change almost immediately.

The big wins are actually in being able to clmul-ise .forward(). Plotty and another table of geomeans versus upstream:

No benchmarks regress either, which is neat.